-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: upgrade jest to incorporate jsdom bugfix #5456
chore: upgrade jest to incorporate jsdom bugfix #5456
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 - Thanks @emyarod!
Deploy preview for carbon-elements ready! Built with commit a4469c5 |
Deploy preview for carbon-components-react ready! Built with commit a4469c5 https://deploy-preview-5456--carbon-components-react.netlify.com |
re-requesting review due to some test refactoring (edited PR description for more context and added relevant tickets for context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment - I see several tests are change to add a <div>
under <body>
. Wondering if it'll be cleaned up once the test finishes?
I remove them in test suites where they are reused, but I can add the line to each modified individual test case https://github.com/carbon-design-system/carbon/pull/5456/files#diff-e6bde0b208435880662cb06911ee85c1R246 |
1b3c84e
to
e1599d5
Compare
Thank you for the update @emyarod! Given you added a clean-up code at the last line of each test case, it won't run if there are any assertions failures, etc. You could use a try ... catch block, but I tend to do: describe('suite', () => {
let div;
it('does the right thing', () => {
div = document.createElement('div');
document.body.appendChild(div);
(Your test)
});
afterEach(() => {
if (div && div.parentNode) {
div.parentNode.removeChild(div);
}
div = null;
});
}); Note: Ensuring clean-up stuffs done after (each) tests avoids flaky tests. |
e1599d5
to
abaa7aa
Compare
abaa7aa
to
797bf4c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the update @emyarod!
…#5456) * chore: upgrade jest * test: adjust for new document.activeElement behavior in jsdom * test: clean up container div Co-authored-by: Josh Black <[email protected]>
This PR upgrades Jest to the latest version (v25.1.0) in order to incorporate a
jsdom
bugfix. The outdatedjsdom
dependency was causing test failures in ourFloatingMenu
component due to an upstream bug which would dispatch afocus
event when thefocus()
method was called on an already focused element, leading to endless cycling betweenfocus
andblur
event handler calls. Discovered while working on #5365refs:
jsdom/jsdom#2621
jsdom/jsdom#2700
jsdom
is also now handlingdocument.activeElement
differently from previous versions, so some tests have been revised to account for that breaking changerefs:
ant-design/ant-design#21185
jsdom/jsdom#2586
jsdom/jsdom#2723
enzymejs/enzyme#2337
Testing / Reviewing
Ensure none of our tests are broken